Reapply "[llvm-exegesis] Exclude loads/stores from aliasing instruction set" (#156735) #159366
Conversation
…on set" (llvm#156735) Move the mayLoad/mayStore checks out of hasMemoryOperands; there are instructions with these properties that don't have operands.
|
@llvm/pr-subscribers-tools-llvm-exegesis Author: Sjoerd Meijer (sjoerdmeijer) ChangesMove the mayLoad/mayStore checks out of hasMemoryOperands; there are instructions with these properties that don't have operands. Full diff: https://github.com/llvm/llvm-project/pull/159366.diff 2 Files Affected:
diff --git a/llvm/test/tools/llvm-exegesis/AArch64/no-aliasing-ld-str.s b/llvm/test/tools/llvm-exegesis/AArch64/no-aliasing-ld-str.s
new file mode 100644
index 0000000000000..65e1203bb275d
--- /dev/null
+++ b/llvm/test/tools/llvm-exegesis/AArch64/no-aliasing-ld-str.s
@@ -0,0 +1,8 @@
+REQUIRES: aarch64-registered-target
+
+RUN: llvm-exegesis -mtriple=aarch64 -mcpu=neoverse-v2 -mode=latency --dump-object-to-disk=%d --opcode-name=FMOVWSr --benchmark-phase=assemble-measured-code 2>&1
+RUN: llvm-objdump -d %d > %t.s
+RUN: FileCheck %s < %t.s
+
+CHECK-NOT: ld{{[1-4]}}
+CHECK-NOT: st{{[1-4]}}
diff --git a/llvm/tools/llvm-exegesis/lib/SerialSnippetGenerator.cpp b/llvm/tools/llvm-exegesis/lib/SerialSnippetGenerator.cpp
index bdfc93e22273b..707e6ee2d434b 100644
--- a/llvm/tools/llvm-exegesis/lib/SerialSnippetGenerator.cpp
+++ b/llvm/tools/llvm-exegesis/lib/SerialSnippetGenerator.cpp
@@ -57,6 +57,12 @@ computeAliasingInstructions(const LLVMState &State, const Instruction *Instr,
continue;
if (OtherInstr.hasMemoryOperands())
continue;
+ // Filtering out loads/stores might belong in hasMemoryOperands(), but that
+ // complicates things as there are instructions with may load/store that
+ // don't have operands (e.g. X86's CLUI instruction). So, it's easier to
+ // filter them out here.
+ if (OtherInstr.Description.mayLoad() || OtherInstr.Description.mayStore())
+ continue;
if (!ET.allowAsBackToBack(OtherInstr))
continue;
if (Instr->hasAliasingRegistersThrough(OtherInstr, ForbiddenRegisters))
|
|
Hi @boomanaiden154, I had to revert the previous commit because I broke all the x86 bots. I have reverted the implementation back to one of my first versions, and have hoisted the extra checks out of After breaking all the x86 bots, I released that there are actually quite a few x86 regression tests that involve running and evaluating latency/throughput measurements and thus running it on hardware. I am on a non-x86 platform, do have the X86 backend built, but realised that I am not running those test when I develop locally, so that was a little lesson learned. |
Thanks for the review, and will do. |
Yeah, those tests are nice to have to test the execution flow, but definitely run extremely sparingly. You need to be on Linux with perf counter access, and quite a few of the bots are virtualized which prevents perf counter access. |
…on set" (llvm#156735) (llvm#159366) Move the mayLoad/mayStore checks out of hasMemoryOperands; there are instructions with these properties that don't have operands. This is relanding 899ee37 with a minor tweak.
| @@ -0,0 +1,8 @@ | |||
| REQUIRES: aarch64-registered-target | |||
|
|
|||
| RUN: llvm-exegesis -mtriple=aarch64 -mcpu=neoverse-v2 -mode=latency --dump-object-to-disk=%d --opcode-name=FMOVWSr --benchmark-phase=assemble-measured-code 2>&1 | |||
There was a problem hiding this comment.
Is %d a standard lit substitution? I don't see it in https://llvm.org/docs/CommandGuide/lit.html#substitutions.
|
The added test is failing on our 2 stage SVE builders, not on single stage for reasons I don't understand. Also it has passed once on one of the builders, so it could be something about our host machines. https://lab.llvm.org/buildbot/#/builders/41/builds/9394 Seems like it's llvm-objdump crashing so it could be a disassembly issue we've not hit before. I won't have time to reproduce it until tomorrow. Let me know if anything in there stands out or there's any reason having SVE or SVE2 on the machine would be a problem. For instance, is this expected? |
Test added by #159366 This is causing objdump to crash more often than not on our 2 stage SVE bots, disabling it and I will investigate tomorrow. Could be the changes in the PR, or a pre-existing codegen or llvm-objdump problem.
|
I disabled the test, will have time to look into it tomorrow. |
I don't why SVE in particular would change things, especially in regards to objdump crashing. My only guess would be that it changes the exegesis codegen which then hits an edge case in objdump.
Yes, that is expected. |
|
Ah, sorry about the late response, I now see I missed a few buildbot messages. |
I assume that %d was meant to be %t. It did work with %d but it was making a file literally called that. Which if nothing else, looks like it's broken. Test remains disabled as I haven't found the cause of the llvm-objdump failure yet. Test added by #159366.
|
Unrelated to llvm-objdump failing, I hammered the test with the first stage build on an AArch64 machine without SVE and sometimes (1 in a 1000 runs maybe) would fail with an error about operands: Just in case that error means anything to you. |
|
Another one: |
Exegesis uses a random seed for generating operands, and it occasionally will trip a case that hasn't been properly handled. I think there are one or two other tests that need fixing because of this. That should be a separate issue from what you're observing with |
|
I enabled the test again. The CHECK-NOT was picking up the file path that llvm-objdump prints, I fixed that. llvm-objdump no longer crashes, so I have no idea why that was happening. We have refreshed all the bot containers since then, that could have "fixed" it. Unfortunately I got more failure notifications like: And I don't think it's worth the spam to leave this enabled. Please fix the errors before enabling it again. |
4 participants
Move the mayLoad/mayStore checks out of hasMemoryOperands; there are instructions with these properties that don't have operands.